[feat]: Scope vueNodesMap per LogicFlow instance to support nested scenarios#2378
[feat]: Scope vueNodesMap per LogicFlow instance to support nested scenarios#2378EralChen wants to merge 2 commits intodidi:masterfrom
Conversation
|
|
About #2377 |
There was a problem hiding this comment.
Pull request overview
This PR scopes Vue node registrations to a specific LogicFlow instance to avoid cross-instance collisions (notably in nested / multi-instance scenarios), by introducing a WeakMap<GraphModel, ...> registry and updating VueNodeView to resolve configs through a scoped getter.
Changes:
- Added per-
GraphModelVue node registry stored in a module-levelWeakMapplusgetVueNodeConfig(type, graphModel). - Deprecated (but still populated) the legacy global
vueNodesMapfor backward compatibility. - Updated
VueNodeViewto load the node component viagetVueNodeConfig(...)instead of the global map.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/vue-node-registry/src/view.ts | Switches runtime config lookup to the new per-instance getter. |
| packages/vue-node-registry/src/registry.ts | Introduces WeakMap-scoped registry, adds getter, keeps legacy global map populated. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let map = vueNodesMaps.get(lf.graphModel) | ||
| if (!map) { | ||
| map = {} | ||
| vueNodesMaps.set(lf.graphModel, map) | ||
| } | ||
| map[type] = entry |
There was a problem hiding this comment.
Both the per-instance registry (map) and the deprecated global vueNodesMap use plain objects indexed by type. If type is not strictly controlled, keys like __proto__/constructor can lead to prototype pollution. Prefer Map<string, VueNodeEntry> (and store it in the WeakMap), or initialize with Object.create(null) and reject dangerous keys.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
That’s strange
it doesn’t seem to be running. Could you please manually add a commit?
…support nested scenarios
EralChen
force-pushed
the
feat/vue_nodes_scoped
branch
from
bdc5da7 to
d391adf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!nodeConfig) return | ||
| const { component } = nodeConfig |
There was a problem hiding this comment.
renderVueComponent() now returns early when getVueNodeConfig() returns undefined. In Teleport mode (isActive()), that can leave a previously-connected teleport entry (items[this.targetId()]) still mounted because disconnect() is only called from unmount(), not here. It can also cause setHtml() to still call startResizeObserver() even though no component was rendered, leaving unnecessary observers attached. Consider cleaning up on this branch (e.g., disconnect/clear any existing mount for this targetId, and avoid starting the resize observer when there is no node config) to prevent stale UI and leaks when a node type isn’t registered for the instance.
| if (!nodeConfig) return | |
| const { component } = nodeConfig | |
| if (!nodeConfig) { | |
| // No registered config for this node type; ensure any existing Teleport mount is cleaned up | |
| if (isVue3 && isActive()) { | |
| disconnect(this.targetId()) | |
| } | |
| return | |
| } | |
| const { component } = nodeConfig | |
| if (!component) { | |
| // Config exists but has no component; also clean up any existing Teleport mount | |
| if (isVue3 && isActive()) { | |
| disconnect(this.targetId()) | |
| } | |
| return | |
| } |
| graphModel: GraphModel, | ||
| ): VueNodeEntry | undefined { |
There was a problem hiding this comment.
getVueNodeConfig() calls vueNodesMaps.get(graphModel) directly. Since this function is exported as a public API, it’s easy for JS callers to accidentally pass undefined/null (or a non-object), which will throw a TypeError from WeakMap.get. Consider adding a small runtime guard (or making graphModel optional and returning undefined when it’s missing) so the API fails safely.
| graphModel: GraphModel, | |
| ): VueNodeEntry | undefined { | |
| graphModel?: GraphModel | null, | |
| ): VueNodeEntry | undefined { | |
| if (typeof graphModel !== 'object' || graphModel === null) { | |
| return undefined | |
| } |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Store Vue node registrations in a
WeakMap<GraphModel, ...>and addgetVueNodeConfig(type, graphModel)so node configs are resolved per LogicFlow instance instead of from a shared global map.Updated
VueNodeViewto read configs through the new scoped getter, which prevents cross-instance registration collisions in multi-instance usage. Kept populating the legacyvueNodesMap(marked deprecated) for backward compatibility.fix(vue-node-registry): isolate node config per LF instanceStore Vue node registrations in a
WeakMap<GraphModel, ...>and addgetVueNodeConfig(type, graphModel)so node configs are resolved per LogicFlow instance instead of from a shared global map.Updated
VueNodeViewto read configs through the new scoped getter, which prevents cross-instance registration collisions in multi-instance usage. Kept populating the legacyvueNodesMap(marked deprecated) for backward compatibility.